feat(roku): add Roku platform support - #3
Conversation
Port the Roku driver from namiml/Maestro (commit ff38700) and integrate it following the fork's Vega/tvOS platform patterns: - ECP client, SSDP/env device discovery, app-ui parser, and key mapping live in maestro-client's maestro/roku/ package (no separate Gradle module) - Devices surface through DeviceService.listRokuDevices() (MAESTRO_ROKU_HOST pin + opt-in MAESTRO_ROKU_DISCOVERY SSDP scan), so the standard --platform roku / --device <ip> selection works with no global CLI flags or TestCommand special-casing - RokuDriver: D-pad input, LIT_ text entry, SceneGraph view hierarchy with scene-absolute bounds, digest-auth screenshots, app-ui-based settle waits - New KeyCodes (Remote Info / Instant Replay / Search) mapped on Roku and Android; Remote Menu maps to Roku's * (options) button - MCP session wiring and Studio TV mode auto-on for Roku - Unit tests for the parser, key mapping, and SSDP parsing - BrightScript demo channel (e2e/roku_demo_app) with navigation/focus flows - FORK.md, README, and AGENTS.md entries Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Validated against a physical Roku Streaming Stick 4K (Roku OS 14). Three fixes, each reproduced and verified on the device: - launchApp is now a cold launch: ECP's /launch resumes an already-running channel with its state intact, so exit to the home screen first (Maestro's launchApp contract; matches VegaDriver's terminate-then-launch) - Screenshot generation: the dev server's multipart parser silently ignores form parts carrying a per-part Content-Length header (which OkHttp's MultipartBody always adds) and also requires an empty 'archive' field — build the form body by hand and do the digest handshake explicitly with an empty-body probe, verifying the 'Screenshot ok' confirmation - Screenshot download: poll /pkgs/dev.jpg's ETag against the pre-generation value (the capture file is written asynchronously) and cache-bust the URL, so a stale prior capture is never returned as the result All three e2e demo-channel flows plus a screenshot flow pass against the real device; roku unit tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rebuild e2e/roku_demo_app to mirror the tvOS/Vega demo apps — same screens,
labels, and testIDs — and mirror all three flows:
- Home menu (menu-navigation / menu-text-input / menu-focus) opening one test
screen at a time, with Back returning Home
- Navigation Test: 2x2 grid (grid-top-left/right/bottom-left/right) covering
all four D-pad directions via an explicit focus-transition map
- Text Input Test: a native Keyboard node (text-field) focused on entry so
inputText (ECP LIT_) and eraseText (Backspace) land in it directly, with a
typed-label echoing the text
- Focus Test: programmatic focus (setFocus on entry) lands on focus-button-2
Driver fix shaken out by the text-input flow: ECP keypress path segments were
form-encoded, so a space became '+' and LIT_+ typed a literal plus — percent-
encode path segments instead ('Hello Roku' now types correctly).
Channel fix found on hardware: creating the hidden TextInputScreen's Keyboard
steals input focus after HomeScreen's init has claimed it, leaving D-pad keys
routed to the invisible keyboard — MainScene re-asserts the Home menu's focus
once all screens exist (focusDefault interface function).
All three mirrored flows pass against a physical Streaming Stick 4K; roku
unit tests (14) pass, including new coverage for the path-segment encoding
and the dev server's digest challenge.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Credit the Roku platform contribution to the Nami team (nami.ml / rku.dev) in the README feature list and the FORK.md platform entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
It follows the Vega/tvOS precedent closely (Platform → DeviceSpec → DeviceService.list* → session managers → AppValidator → Studio TV mode), adds no global CLI flags, and doesn't special-case TestCommand. Docs are good.
Verified locally: :maestro-client:compileKotlin + :maestro-cli:compileKotlin pass, and all 14 Roku unit tests pass. Haven't validated it yet on my own hardware.
Four things to comment on beforehand though:
High
1. Failed ECP commands never fail the flow — RokuEcpClient.kt:396-407
ecpPost logs a warning and returns on failure. Every tapOn, pressKey, inputText, backPress, swipe and eraseText goes through it. This isn't hypothetical — the class doc itself (RokuEcpClient.kt:27-28) notes that recent Roku OS returns 403 on input commands when ECP access isn't Permissive, while /query/app-ui keeps working. That's exactly the split where the hierarchy reads fine, every keypress silently no-ops, and a flow goes green having never touched the device. ecpPost should throw.
2. executeWithRetry discards the HTTP status — RokuEcpClient.kt:429-450
OkHttp's execute() doesn't throw on non-2xx — it returns a Response for every status and only throws on transport failures. So a 403 lands in the if (response.isSuccessful) false branch, the response is closed, and the status is dropped at line 438. lastException is therefore always null in that case, and the final log
logger.warn("ECP request to ${request.url} failed after $maxRetries attempts", lastException)fires with no cause and no status code — the single most likely setup error is unreportable. Separately, the 50ms backoff only runs in the catch, so a 403 burns all three attempts back-to-back with zero delay.
3. A launch that never happens passes — RokuDriver.kt:111-114
if (!appActive) {
logger.warn("App $appId did not become active within timeout")
return
}Returning normally after a failed launch means subsequent asserts fail against whatever screen happens to be up, with a confusing message. Should throw.
4. visible="false" nodes stay in the hierarchy as visible — RokuAppUIParser.kt:87,149
Invisibility is encoded as enabled = visible && opacity > 0, but the node keeps real bounds and stays in the tree. ViewHierarchy.isVisible (ViewHierarchy.kt:40-50) only checks bounds + topmost-at-center — it never consults enabled. So assertVisible matches hidden nodes and assertNotVisible fails on them. RokuAppUIParserTest's "invisible nodes are disabled" locks this in. Same class of bug as the one commit 3 hit inside the channel (hidden TextInputScreen keyboard stealing focus). Fix: skip visible="false" subtrees, or omit their bounds.
Medium
5. RTA_LAUNCH=1 contradicts the cold-launch contract — RokuEcpClient.kt:94
The comment reads "Prevent restart if already running", which is precisely what commit 2 worked around by exiting to Home first. For any channel that honors the flag the two fight. It's also injected into openLink deep links (RokuDriver.kt:254), leaking an unexpected param into user channels.
6. swipe and scrollVertical disagree — RokuDriver.kt:198-237
scrollVertical() sends Down; swipe(SwipeDirection.UP) sends Up. On Vega, scrollVertical() is swipe(UP) (VegaDriver.kt:108-110), so Roku is both internally inconsistent and inverted relative to every other platform.
7. Shared DocumentBuilderFactory, no XXE hardening — RokuEcpClient.kt:473
DocumentBuilderFactory isn't thread-safe for newDocumentBuilder(), and there's no disallow-doctype-decl on XML sourced over the network. Low practical risk on a LAN, but it's a one-liner.
Low
RokuAppUIParser.kt:101,106—parseArraycan return a 1-element array, sotranslation[1]/bounds[1]can throwArrayIndexOutOfBounds. Inconsistent with thebounds.size >= 4guard six lines below.RokuEcpClient.kt:340-343—close()shuts the dispatcher but skipsconnectionPool.evictAll().RokuEcpClient.kt:368—digestNonceCountis global; RFC 2617 wantsncrestarting at 1 per nonce. Roku likely doesn't care.RokuDriver.kt:74— thedeviceInfo()fallback never caches into the field, so it re-queries every call.RokuAppUIParser.kt:84setssubtypefromnodeName, ignoring the actualsubtypeattribute its own docstring shows at line 18.roku-text-input-flow.yaml— bareeraseTextdefaults to 50 chars → 50 sequential keypresses at 100ms ≈ 5s. Worth an explicit count.
Fork conventions
.claude/skills/update-from-upstream/SKILL.md:25— the fork-commit table has a Vega row but no Roku row.verify-fork-stackwill report drift on both.
…en nodes Addresses the High findings in the plexinc#3 review. All four are the same class of bug: the driver reported a problem to the log and let the flow continue, so a run could pass green having never driven the device. - ecpPost now throws. A device with ECP network access set to anything but Permissive serves /query/app-ui while answering input commands with 403, so every tapOn/pressKey/inputText/backPress/swipe/eraseText silently no-opped while the hierarchy still read fine. - executeWithRetry keeps the HTTP status. OkHttp returns a Response for every status and only throws on transport failures, so a 403 landed in the unsuccessful branch with lastException left null — the likeliest setup error was unreportable. Failures now carry the status in a RokuEcpException, 403 and 401 carry setup hints, 4xx skips the retries it could never satisfy, and the backoff moved out of the catch so a non-2xx no longer burns three attempts back to back. Queries stay tolerant: callers poll them and report an unavailable hierarchy themselves. - launchApp throws when the channel never becomes active, instead of returning into asserts against whatever screen happens to be up. - The parser drops nodes the device isn't rendering (visible="false", opacity="0") along with their subtrees. Encoding invisibility as `enabled = visible && opacity > 0` did nothing: ViewHierarchy.isVisible only consults bounds, so assertVisible matched hidden nodes and assertNotVisible failed on them. Roku unit tests 14 -> 20, covering each new failure path off MockWebServer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the Medium findings in the plexinc#3 review. - Remove the RTA_LAUNCH=1 launch parameter. Nothing in the fork reads it; on a roku-test-automation channel it asks for no restart, which is exactly what the exit-to-Home cold launch exists to force; and on any other channel it is an uninvited parameter riding along with the flow's own openLink deep link. launchChannel now sends only the caller's parameters, and omits the query string entirely when there are none. - Invert swipe directions. A swipe names where the content goes, so on a focus-driven UI every direction flips: swipe(UP) reveals what is below and therefore presses Down. Roku had swipe(UP) pressing Up, inverted relative to VegaDriver (where scrollVertical() *is* swipe(UP)) and to web (where swiping up increases scrollY). The mapping now lives in RokuKeyMapping so a unit test can lock it without hardware, the coordinate overload converts its delta to a SwipeDirection and delegates, and scrollVertical() is swipe(UP) as on Vega — emitting the same five Down presses it always did. - Harden the XML parser: FEATURE_SECURE_PROCESSING, disallow-doctype-decl, no external entities, no XInclude, on network-sourced XML. DocumentBuilderFactory is not thread-safe for newDocumentBuilder() and the driver polls the hierarchy off more than one thread, so builder creation moved under a lock. Roku unit tests 20 -> 25. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the Low findings and the fork-conventions note in the plexinc#3 review. - parseArray takes a minSize and returns null below it (2 for translation, 4 for bounds), so the positional reads can't run off a short array; the downstream `bounds.size >= 4` guard collapses into the null check, putting the arity rule in one place. Its NumberFormatException catch also returned null as the expression value rather than from the function. - close() evicts both connection pools, not just the dispatchers. - The digest nonce count restarts at 1 for each new server nonce, per RFC 2617, instead of climbing for the client's lifetime. - deviceInfo() caches the result of its retry. contentDescriptor reads that field on every hierarchy dump, so an uncached fallback cost an extra ECP round trip per command. - The parser reports the subtype attribute the device sent, falling back to the element name and to Group only for a bare RenderableNode. - roku-text-input-flow erases an explicit 12 characters: every one is a separate ECP Backspace keypress, so the default of 50 spent ~5s clearing 10 characters. - Add the Roku row to the update-from-upstream fork-commit table, which verify-fork-stack reconciles against the stack one-to-one. Roku unit tests 25 -> 27. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — this was a sharp review, and the High section named a real theme we'd missed: four separate places where the driver logged a problem and let the flow keep going. All fourteen findings plus the fork-table note are fixed in three commits, split to match your sections. High (
Medium ( Low (
Roku unit tests are 14 → 27, all green; |
Proposed changes
copilot:summary
Adds Roku as a supported platform (
--platform roku), following the integration patterns established by the tvOS and Vega ports. Contributed by teams Nami and rku.dev.drivers/RokuDriver.kt+maestro/roku/): Roku devices are driven over the External Control Protocol (an HTTP API on device port 8060) — no on-device agent. View hierarchy comes from/query/app-ui(SceneGraph XML parsed into scene-absolute bounds), input is D-pad keypresses (tapOnsends Select; swipes/scrolls become directional presses), text is typed via character-by-characterLIT_keypresses, and screenshots go through the dev web server (digest auth).launchAppis a cold launch: an already-running channel is exited to the home screen first.DeviceService.listRokuDevices()surfaces devices through the normal listing/selection path — aMAESTRO_ROKU_HOSTpin (fast reachability probe) plus an opt-in SSDP LAN scan (MAESTRO_ROKU_DISCOVERY=true). No new global CLI flags and noTestCommandspecial-casing.Platform.ROKU,DeviceSpec.Roku,RokuLocale, session managers (CLI + MCP), pickers,start-device,AppValidator.Remote Info(the*options button),Remote Instant Replay,Remote Search— mapped on Roku and Android;Remote Menumaps to Roku's options button.e2e/roku_demo_app/) mirroring the tvOS/Vega demo apps — same screens, labels, and testIDs (Home menu, 2×2 navigation grid, native-Keyboard text input, programmatic-focus test) — with three mirrored flows undere2e/workspaces/roku_demo_app/.Notable device-behavior quirks handled (all reproduced on hardware): the dev server's multipart parser ignores form parts carrying a per-part
Content-Lengthheader (so the screenshot form is assembled by hand with an explicit digest handshake), the screenshot file is written asynchronously (the download polls its ETag against the pre-generation value), and ECP keypress path segments must be percent-encoded (a form-encoded space would type a literal+).Testing
maestro-client/src/test/java/maestro/roku/): app-ui parser (bounds math, translation offsets, RowListItem duplicate-drop, focus state), key mapping, SSDP response parsing, ECP path-segment encoding, and the dev server's digest challenge../gradlew :maestro-client:test --tests "maestro.roku.*"maestro test --platform roku e2e/workspaces/roku_demo_app/), covering launch, all four D-pad directions, focus asserts (focused: true/falseby id), programmatic focus,inputTextwith spaces/capitals,eraseText, Back-driven screen returns, andtakeScreenshot(captures visually verified against the asserted screen state).list-devicesshows a pinned Roku (and degrades gracefully in ~5s when the host is unreachable);start-device --platform rokuwithoutMAESTRO_ROKU_HOSTproduces an actionable error.FlutterWebSemanticsIdentifierTestfailure exists onmainindependently of this change).Issues fixed
None — new platform capability.